Auto generate Event Ids in the logging source gen #87892
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue Detailsnull
|
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
| @@ -298,8 +308,7 @@ public IReadOnlyList<LoggerClass> GetLogClasses(IEnumerable<ClassDeclarationSynt | |||
| } | |||
|
|
|||
| // ensure there are no duplicate event ids. | |||
| // LoggerMessageAttribute has constructors that don't take an EventId, we need to exclude the default Id -1 from duplication checks. | |||
| if (lm.EventId != -1 && !eventIds.Add(lm.EventId)) | |||
| if (!eventIds.Add(lm.EventId)) | |||
There was a problem hiding this comment.
Does this mean someone could get a diagnostic because the generated ID happened to conflict?
There was a problem hiding this comment.
Does this mean someone could get a diagnostic because the generated ID happened to conflict?
IMO this is reasonable to happen. At that time users can opt-in providing their own id. This will better than just allowing the same Id for multiple logging methods. In same time I want to avoid generating different id in case of collisions as I want to have the generated Id value is predictable and consistent.
There was a problem hiding this comment.
@stephentoub any more thoughts here?
There was a problem hiding this comment.
I realize the failure is very unlikely to occur, but if it does occur, it will be quite confusing for the customer. Imagine if the C# compiler sometimes just failed to compile a C# source file because it has a hash collision in the names of the symbols in the type.
What else can we do?
If there's a conflict, could we just increment the id value by 1 and try again until we find an unused id?
There was a problem hiding this comment.
If there's a conflict, could we just increment the id value by 1 and try again until we find an unused id?
This is what I am trying to avoid. This means we can generate a different Id when other parts of the code change.
Imagine a scenario where we generate Id without any conflict and this code ships. Later in v2, more logger methods added cause a conflict which causes us to regenrate the Id for the methods which previously shipped. This kind of breaking changes if anyone dependent on the Id generated in v1.
I think getting diagnostics when Id collision occurs is not that bad especially it is unlikely to happen.
There was a problem hiding this comment.
I think such a diagnostic is problematic. We were willing to live with duplicate IDs when they defaulted to -1, I'm not sure why we're not willing to live with them when they default to something else.
I'd be fine with:
- Just not auto-generating IDs, and if you care about IDs, assign one.
- Auto-generating IDs but not warning when they conflict.
I don't think we should auto-generate IDs and warn when they conflict. Then harmless activities like tweaking a logging method name can start producing new seemingly unrelated and haphazard warnings.
There was a problem hiding this comment.
Ok, I applied no. 2 in the commit b1d5a89 (#87892)
ghost
locked as resolved and limited conversation to collaborators
No description provided.